Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking Change: Support ESM #22

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Breaking Change: Support ESM #22

wants to merge 4 commits into from

Conversation

jeswr
Copy link
Member

@jeswr jeswr commented Aug 8, 2024

Breaking changes:

  • Files now use named exports rather than default exports
  • Deleted MergeIterator which is the same as UnionIterator in AsyncIterator
  • Re-organised directory structure; built CJS files are now in dist/ and built ESM files are in esm/
  • nestedLoopJoin and dynamicNestedLoopJoin are now functions that use AsyncIterator methods rather than re-implementing existing logic in the AsyncIterator library. These functions could be made even simpler and faster if someone wants to go to the effort of landing this multimap implementation in asynciterator. It may be worth considering the deletion of these two functions from the asyncjoin library entirely.

@@ -11,23 +11,41 @@
"asynciterator": "^3.9.0"
},
"devDependencies": {
"@types/node": "^22.1.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this devDependency is required to overcome the following error from AsyncIterator.

This may mean that @types/node or @types/event needs to be added to the AsyncIterator dependencies.

$ yarn run build
yarn run v1.22.22
$ npm run build:cjs && npm run build:esm

> asyncjoin@1.2.3 build:cjs
> tsc

join/HashJoin.ts:21:19 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

21         this.left.on('error', (error) => this.destroy(error));
                     ~~

join/HashJoin.ts:21:32 - error TS7006: Parameter 'error' implicitly has an 'any' type.

21         this.left.on('error', (error) => this.destroy(error));
                                  ~~~~~

join/HashJoin.ts:22:20 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

22         this.right.on('error', (error) => this.destroy(error));
                      ~~

join/HashJoin.ts:22:33 - error TS7006: Parameter 'error' implicitly has an 'any' type.

22         this.right.on('error', (error) => this.destroy(error));
                                   ~~~~~

join/HashJoin.ts:28:24 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

28             this.right.on('readable', () => this.readable = true);
                          ~~

join/HashJoin.ts:29:24 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

29             this.right.on('end', () => { if (!this.hasResults()) this._end(); });
                          ~~

join/HashJoin.ts:32:19 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

32         this.left.on('end', allowJoining);
                     ~~

join/HashJoin.ts:34:14 - error TS2339: Property 'on' does not exist on type 'HashJoin<S, H, T>'.

34         this.on('newListener', (eventName) =>
                ~~

join/HashJoin.ts:34:33 - error TS7006: Parameter 'eventName' implicitly has an 'any' type.

34         this.on('newListener', (eventName) =>
                                   ~~~~~~~~~

join/HashJoin.ts:43:19 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

43         this.left.on('readable', () => this._addDataListenerIfNeeded());
                     ~~

join/HashJoin.ts:110:19 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

110         this.left.on('data', addItem);
                      ~~

join/SymmetricHashJoin.ts:16:14 - error TS2339: Property 'on' does not exist on type 'SymmetricHashJoin<S, H, T>'.

16         this.on('end', () => this._cleanup() );
                ~~

join/SymmetricHashJoin.ts:23:19 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

23         this.left.on('error', (error) => this.destroy(error));
                     ~~

join/SymmetricHashJoin.ts:23:32 - error TS7006: Parameter 'error' implicitly has an 'any' type.

23         this.left.on('error', (error) => this.destroy(error));
                                  ~~~~~

join/SymmetricHashJoin.ts:24:20 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

24         this.right.on('error', (error) => this.destroy(error));
                      ~~

join/SymmetricHashJoin.ts:24:33 - error TS7006: Parameter 'error' implicitly has an 'any' type.

24         this.right.on('error', (error) => this.destroy(error));
                                   ~~~~~

join/SymmetricHashJoin.ts:26:19 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

26         this.left.on('readable', () => this.readable = true);
                     ~~

join/SymmetricHashJoin.ts:27:20 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

27         this.right.on('readable', () => this.readable = true);
                      ~~

join/SymmetricHashJoin.ts:30:19 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

30         this.left.on ('end', () => { if (!this.hasResults()) this._end(); });
                     ~~

join/SymmetricHashJoin.ts:31:20 - error TS2339: Property 'on' does not exist on type 'AsyncIterator<S>'.

31         this.right.on('end', () => { if (!this.hasResults()) this._end(); });
                      ~~


Found 20 errors in 2 files.

Errors  Files
    11  join/HashJoin.ts:21
     9  join/SymmetricHashJoin.ts:16

@jeswr jeswr requested a review from rubensworks August 8, 2024 05:43
@jeswr
Copy link
Member Author

jeswr commented Aug 8, 2024

In fact I'm inclined to go a step further and suggest that this library be deprecated, and that the HashJoin and SymmetricHashJoin be moved into the core of comunica

@rubensworks
Copy link
Member

I agree in idea with all of the changes!

nestedLoopJoin and dynamicNestedLoopJoin are now functions that use AsyncIterator methods rather than re-implementing existing logic in the AsyncIterator library. These functions could be made even simpler and faster if someone wants to go to the effort of landing RubenVerborgh/AsyncIterator#50 (comment) in asynciterator. It may be worth considering the deletion of these two functions from the asyncjoin library entirely.

Given the invasiveness of this change, we'll need to have a close look at the impact in comunica performance-wise. Both base comunica and link traversal comunica make very heavy usage of the nested loop join (does not use dynamic nested loop join at all). So even the tiniest change in implementation could have a very big impact on them.

So if you want to have a look at this impact, that would be great, and I can give you instructions.
If not, it might be better if we skip this part of the PR.

@rubensworks
Copy link
Member

In fact I'm inclined to go a step further and suggest that this library be deprecated, and that the HashJoin and SymmetricHashJoin be moved into the core of comunica

Sure, that also works :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants